CODAP-1505: store point shape per legend category - #2692
Conversation
Add the model layer for point shapes: each legend category can carry its own shape, and a display with no legend attribute carries a single one. Nothing reads these yet -- the controls come with CODAP-1506 and the rendering with CODAP-1504. Per-category shape lives on the attribute's CategorySet, next to per-category color, so two displays using the same legend attribute agree by construction. The single shape lives on DisplayItemDescriptionModel next to point color. That pairing mirrors color exactly, which is what the existing per-category color setter already established. The default is stored as absence: assigning circle removes the entry rather than recording it. A document therefore carries only the shapes a user chose, and one that never used the feature is byte-for-byte unaffected. V2 round trip ------------- V2 has no field for point shape, so both halves travel in v3 extension namespaces: per-category shapes on the attribute, the single shape on the graph component. Both survive a v3 -> v2 -> v3 round trip. A v2 re-save drops them, which is lossy but harmless. They deliberately do NOT ride in _categoryMap, which would be the obvious home next to colors. V2's updateCategoryMap treats every key of that map except __order, stroke-color and stroke-transparency as a category: it appends unknown keys to __order and writes the result back out. A shapes key there would appear as a phantom category in the user's legend and be persisted by v2 itself. An unknown key in the attribute's v3 namespace is merely mixed onto the SproutCore model and dropped by its toArchive allowlist, so v2 is unaffected. A test pins this, since the safe-looking change is the harmful one. Imported shapes are not filtered against the categories present in the data, and so differ from colors, which are. Every shape entry is a deliberate user assignment, whereas some paths assign colors automatically by category position -- which is why the color import keeps only a color differing from the one that position would have produced. Shapes have no generated noise to age out, so a category whose cases are deleted and later restored keeps its shape. V2 does the same, for the reason its own comment gives. Provisional promotion --------------------- A category set stays provisional until the user changes something that has to persist, at which point it is promoted into the document. Shapes join moves and colors as such a change: assigning a shape can be the only thing a user does, so the promotion predicate has to count it for the shape to be saved at all. Also remove CategorySet.userActionNames, which named the changes that promote a set but was read by nothing. Its test asserted against the list rather than the behavior; the equivalent coverage now runs against the promotion predicate itself, where moves, colors and shapes are checked together. Document that promotion replaces the category-set instance, so a reference held across a modification goes stale and later writes are silently lost. The per-category setters re-resolve for this reason. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ctmv18kjkPWnUq5bek5nrH
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## CODAP-1506-format-panel-restructure #2692 +/- ##
=======================================================================
- Coverage 87.89% 87.87% -0.03%
=======================================================================
Files 818 819 +1
Lines 47231 47280 +49
Branches 11948 12097 +149
=======================================================================
+ Hits 41513 41546 +33
- Misses 5702 5718 +16
Partials 16 16
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
codap-v3
|
||||||||||||||||||||||||||||
| Project |
codap-v3
|
| Branch Review |
leads-point-shapes
|
| Run status |
|
| Run duration | 03m 45s |
| Commit |
|
| Committer | Kirk Swenson |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
|
|
0
|
|
|
0
|
|
|
0
|
|
|
0
|
|
|
4
|
| View all changes introduced in this branch ↗︎ | |
There was a problem hiding this comment.
🟡 Changes recommended
Default shape fields alter untouched V3 snapshots, and reserved category names can lose shapes during V2 round trips.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds model-layer storage and V2 round-trip support for point shapes.
Changes:
- Adds point-shape types, defaults, and validation.
- Stores shapes globally or per legend category.
- Adds V2 extensions, notifications, undo strings, and tests.
File summaries
| File | Description |
|---|---|
v3/src/v2/v2-document-round-trip.test.ts |
Tests category-shape round trips. |
v3/src/v2/codap-v2-types.ts |
Adds graph shape extension. |
v3/src/v2/codap-v2-type-utils.ts |
Exports non-default graph shapes. |
v3/src/v2/codap-v2-data-set-importer.ts |
Imports attribute shape maps. |
v3/src/v2/codap-v2-data-context-types.ts |
Types category-shape extensions. |
v3/src/utilities/translation/lang/en-US.json5 |
Adds undo/redo strings. |
v3/src/utilities/point-shape-utils.ts |
Defines supported shapes. |
v3/src/utilities/point-shape-utils.test.ts |
Tests shape utilities. |
v3/src/models/shared/data-set-metadata.ts |
Promotes shape-modified category sets. |
v3/src/models/shared/data-set-metadata.test.ts |
Tests provisional promotion. |
v3/src/models/data/v2-category-set-importer.ts |
Restores validated category shapes. |
v3/src/models/data/v2-category-set-importer.test.ts |
Tests shape imports. |
v3/src/models/data/category-set.ts |
Stores per-category shapes. |
v3/src/models/data/category-set.test.ts |
Tests category shape behavior. |
v3/src/data-interactive/point-shape-v2-export.test.ts |
Tests attribute V2 export. |
v3/src/data-interactive/data-interactive-type-utils.ts |
Exports attribute shape maps. |
v3/src/components/graph/v2-graph-importer.ts |
Imports graph point shapes. |
v3/src/components/graph/v2-graph-exporter.ts |
Exports graph point shapes. |
v3/src/components/graph/v2-graph-exporter.test.ts |
Tests graph shape round trips. |
v3/src/components/graph/models/graph-data-configuration-model.test.ts |
Tests legend shape accessors. |
v3/src/components/data-display/models/display-item-description-model.ts |
Stores global point shape. |
v3/src/components/data-display/models/display-item-description-model.test.ts |
Tests global shape storage. |
v3/src/components/data-display/models/data-configuration-model.ts |
Adds legend shape accessors. |
v3/src/components/data-display/data-display-notifications.ts |
Adds shape-change notifications. |
Review details
- Files reviewed: 24/24 changed files
- Comments generated: 5
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Store the display's shape as absence at the default. types.optional with a
default materializes the field in every snapshot, so opening and saving a
document that never used shapes would change it. maybe leaves it absent, the
getter already resolves that to circle, and the setter now clears it when the
default is chosen -- the same absence-means-default rule the per-category
shapes already follow.
Build the exported and imported shape records with Object.fromEntries rather
than by assignment. A category value of `__proto__` assigned with `map[key] =`
sets the prototype instead of defining an own property, so that category's shape
vanished in both directions. Category values come from the data and can be any
string. Note colorMap has the same pattern and the same latent problem; left
alone as pre-existing rather than widened into this story.
Add tests for changePointShapeNotification, which the ten adjacent notification
helpers already have and this one did not.
The remaining comment is declined: the shapes map does materialize as `{}` in
every CategorySet snapshot, but so does the colors map beside it, so the
snapshot was never unaffected by an empty map. Omitting it needs a
snapshotProcessor and would make shapes behave differently from its sibling.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Ctmv18kjkPWnUq5bek5nrH
Category values come from the data and can be any string, including ones that mean something to a plain JavaScript object. Two faults, both pre-existing, found while fixing the same class of bug for shapes. Writing: `map["__proto__"] = color` sets the prototype rather than defining an own property, so that category's color vanished from colorMap and from the v2 export built out of it. Reading: for a category named `constructor` or `toString` with no entry, a bare lookup returns the inherited member. That is truthy and is not a color, so the v2 importer dereferenced `.colorString` to undefined and handed it to colord -- inventing a color for a category the document said nothing about. colorMap is now null-prototyped and built from entries, which closes both. The importer guards its lookup with hasOwnProperty and collects entries rather than assigning into an object. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ctmv18kjkPWnUq5bek5nrH
|
Reviewing this stack — four PRs, each based on the one below it, merging bottom-up into
CODAP-1506 is split across #2691 and #2693, so approving #2691 does not complete that story — it should not leave code review until #2693 is approved too. The other two map one-to-one. Some things are introduced early and deliberately revised further up the stack. Rather than have you spend time on them twice, each PR notes what is superseded. Superseded in #2696 — please review these as they stand here and don't file them against this PR:
Worth your attention here: the v2 round-trip strategy. Shapes travel in the attribute's Out of scope by design, each with a ticket: PIXI renderer (CODAP-1509), legend keys (CODAP-1507), residual plot (CODAP-1526). |
tealefristoe
left a comment
There was a problem hiding this comment.
Looks good 👍
Main feedback:
- Worth looking through the comments and pruning duplicates and unnecessary/distracting comments. I flagged a few, but not all.
- Probably outside of your control, but when will we stop supporting v2? Something like a quarter of the complexity of this PR comes from supporting v2.
- It's probably too late to change it, but I'm not sure about explicitly removing the default shape from the category map.
- If I go default -> star -> circle, I think there's a good chance I want the shape to be circle, not the default, which might change in the future, and which as a user I probably have no concept of.
- It would be simpler to just save the default shape when the user sets it.
Maybe there's some subtle reasons to do this that I'm missing?
Drop a claim that is not true. The note on importing shapes ended by saying v2 does the same thing for the same reason, but v2 has no point shapes at all -- which is why they travel in the v3 namespace rather than the category map. The sentence described what v2 does with colors. Prune comments that repeat each other or the code: the note on the shapes map that colors does without, the reason for keeping a category set that the condition below it already gives, and a comment duplicated between the v2 type and the props that build it, reworded between the two for no reason. The note about `__proto__` said the same thing at length in two files and now says it once in a line at each. Test `isPointShape` against a set rather than scanning the list. Move the test for a shape on a category named `__proto__` in with the other reserved-key cases, where its color counterpart already lives. Drop a test that asserted a category set is created when only shapes are restored. The test above it passes only shapes and reads a property off the result, so it cannot pass unless the set was created; the intent moves into its name and comment. Assert the default through the constant rather than by naming circle, since the assertion is about the round trip rather than about which shape is the default. Fix the indentation of three tests, which had drifted from the file. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A pass for the duplication the review asked about beyond what it flagged. Why shapes are exported in the attribute's v3 namespace rather than in _categoryMap was explained four times, twice at length. It is stated once now, on the v3 field itself, since that field exists for no other reason. The export test points at it, the round-trip assertion says nothing the test's own name does not, and the exporter says only what it omits. That promoting a category set replaces the instance was stated at the promotion and again where callers resolve one. The caller-facing one is the one that changes what someone writes, so it keeps the explanation. The shapes list described what it is before saying where it lives. Only the second is invisible from the code. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Thanks — a genuinely useful pass. Everything actionable is in On comments (1). Taken — and since you said you'd flagged a few but not all, I made a pass for the rest rather than just the ones you named. Three more duplications you hadn't flagged:
178 → 155 comment lines across the PR. The cuts are repetition, not explanation. Worth noting two of your flags turned out to be wrong comments rather than merely wordy ones — one asserted something untrue about how v2 handles shapes. That's the better argument for pruning: length was hiding errors. On v2 (2). Kirk's answer, which reframes it — we're not supporting the v2 application, we're supporting the v2 file format, which is part of the plugin API and effectively permanent. Concretely: StoryBuilder stores V2 document states, so without the round-trip, point shapes would silently disappear from any StoryBuilder moment. The complexity buys plugin compatibility rather than deferring a sunset. On removing the default from the category map (3). You're right, and it's already fixed one PR up — Copilot reached the same conclusion on #2696 from the other direction. Your So this PR does show the behavior you're questioning, and it doesn't survive the stack. |
Part of the LEADS point-shapes feature. This is the model layer only — nothing
reads these values yet. The controls come with CODAP-1506 and the rendering with
CODAP-1504.
Stacked on #2691. Base is
CODAP-1506-format-panel-restructure, so reviewthat one first. Once it merges I'll retarget this to
leads-point-shapesandthe diff will shrink to just this story.
Where shape lives
Per-category shape goes on the attribute's
CategorySet, next to per-categorycolor, so two displays using the same legend attribute agree by construction.
The single no-legend shape goes on
DisplayItemDescriptionModel, next to pointcolor. That mirrors how color already works.
The default is stored as absence — assigning
circleremoves the entry ratherthan recording it. A document carries only the shapes a user chose, and one that
never used the feature is byte-for-byte unaffected.
The v2 decision worth reviewing
V2 has no field for point shape, so both halves ride v3 extension namespaces:
per-category on the attribute, the single shape on the graph component. Both
survive a v3 → v2 → v3 round trip; a v2 re-save drops them, which is lossy but
harmless.
They deliberately do not ride in
_categoryMap, which looks like theobvious home next to colors. I checked V2 on
masterbefore ruling it out —attribute_model.js:486:updateCategoryMaptreats every other key as a category: it appends unknownkeys to
__order, thentoArchivewrites the result back. A shapes key therewould surface as a phantom category in the user's legend and be persisted by
v2 itself. By contrast an unknown key in the attribute's
v3namespace ismerely mixed onto the SproutCore model by
DG.Attribute.createand dropped bytoArchive's allowlist, so v2 is unaffected.There's a test pinning this, because the harmful change is the one that looks
safe.
Imported shapes are not filtered, unlike colors
Colors are filtered to categories present in the data; shapes are not. The
difference is that every shape entry is a deliberate user assignment, whereas
some paths assign colors automatically by category position — which is why the
color import keeps only a color differing from the one that position would have
produced. Shapes have no generated noise to age out, so a category whose cases
are deleted and later restored keeps its shape. V2 does the same, for the reason
its own comment gives.
Provisional promotion
A category set stays provisional until the user changes something that must
persist. The predicate watched moves and colors; shapes join it, since assigning
a shape can be the only thing a user does.
This also removes
CategorySet.userActionNames, which named the promotingchanges but was read by nothing. Its test asserted against the list rather than
the behavior; equivalent coverage now runs against the promotion predicate
itself, where moves, colors and shapes are checked together.
Promotion rebuilds the set from a snapshot and so replaces the instance — a
reference held across a modification goes stale and later writes are silently
lost. That's now documented at both
getCategorySetandpromoteProvisionalCategorySet; the per-category setters re-resolve for thisreason.
Two deviations from the ACs
V3.notDG.The AC specifiesDG.Undo.graph.changePointShape, butDG.*is for strings inherited from v2and shape is new in v3, so these are
V3.Undo/Redo.graph.changePointShape.getLegendShapeForCaseis not here. The per-case resolver mirroringgetLegendColorForCaseis a rendering concern; CODAP-1504 adds it where it'sused.
Testing
29 new tests. The full-document round-trip tests are the ones that earned their
keep — they caught the promotion gap that every unit test missed, because unit
tests read back through the same in-memory object.
Mutation-tested; each of these fails at least one test: writing shapes into
_categoryMap, filtering imported shapes to present categories, acceptingunrecognized shape values, storing the default instead of omitting it, dropping
shapes from the promotion predicate, dropping
movesfrom it, and having thegraph importer ignore
v3.pointShape.npm run build:tsc,npm run lint, and the full suite (3805 tests / 357suites) all pass.
🤖 Generated with Claude Code
https://claude.ai/code/session_01Ctmv18kjkPWnUq5bek5nrH